Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add umask option to Fluentd system configuration & Fix Tests #4829

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add umask option to Fluentd system configuration & Fix Tests #4829

wants to merge 3 commits into from
+115 −2

Conversation

egemenkus
Copy link

Which issue(s) this PR fixes:
Fixes #4816

What this PR does / why we need it:
This PR introduces an umask option in Fluentd system configuration.
Previously, users had to use the --umask command-line argument, which was inconvenient for service deployments and containerized environments.
With this PR, users can now define umask directly in fluent.conf, making it easier to manage in service configurations.

Additionally, this PR fixes previous test issues and reopens the PR after correcting an earlier mistake. The previous PR contained incorrect assertions in the test cases, which have now been corrected to ensure that:

The configured umask value is correctly applied.
New files are created during testing, and their permissions are checked to verify that the umask is applied properly.
Invalid umask values raise a Fluent::ConfigError as expected.

Based on my tests, I believe this implementation will work as expected in the actual program. However, if any issues are found during the review process, I am more than happy to make the necessary adjustments.

Docs Changes:
No documentation changes required.

Release Note:
Added umask option to Fluentd system configuration.
Fixed test cases related to umask handling and file permissions.
Implemented additional tests to verify umask application by creating new files and checking permissions.

@egemenkus
Added umask support to system configuration
68edd0d 
- Implemented 'umask' option in system config to address fluent#4810.
- Users can now define 'umask' in Fluentd configuration instead of CLI args.
- Improves usability for services and container images by removing reliance on '--umask' argument.

Signed-off-by: kushynoda <egemen.utku3@gmail.com>
@egemenkus
feat(system): Add umask option to Fluentd system configuration
86367ed 
Previously, Fluentd users had to use the --umask command-line argument
to set the umask, which was inconvenient for service deployments and
containerized environments. This commit introduces a umask option in
the Fluentd system configuration, allowing users to define umask
directly within fluent.conf.

Additionally, this commit fixes test cases related to umask handling:
- The previous tests incorrectly asserted the expected umask values.
- Adjusted tests to ensure that new file permissions are correctly
  applied according to the configured umask.
- Added a test to verify that an invalid umask value raises the expected
  Fluent::ConfigError.

Fixes fluent#4816

Release Note:
- Added umask option to Fluentd system configuration.

Signed-off-by: kushynoda <egemen.utku3@gmail.com>
@egemenkus
refactor(tests): Remove redundant umask test cases
c5119b2 
Signed-off-by: kushynoda <egemen.utku3@gmail.com>
@daipom daipom self-requested a review February 17, 2025 02:30
@daipom
Copy link
Contributor

daipom commented Feb 17, 2025

@egemenkus Thanks for this improvement!
I will review this.

daipom
daipom requested changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@daipom daipom left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egemenkus Sorry for my late response.
In the first place, I found a bug in --umask options.

It appears that we need to fix the bug first.
As commented next, even if #4836 is merged, the umask will not be applied in the normal launch.

It will be applied when using --daemon option.
It is the specification of ServerEngine.

Based on the above, could you please fix this PR entirely as follows?

  • Add umask option to SystemConfig.
  • Assuming #4836, replace the passing value with the value of SystemConfig's option.
    • It would be better to start the fix after #4836 is merged.
    • SystemConfig fetches command line options as follows. (So we can replace the command line value with the SystemConfig value)

      • fluentd/lib/fluent/supervisor.rb

        Lines 1168 to 1184 in 644c921

        def build_system_config(conf)
        system_config = SystemConfig.create(conf, @cl_opt[:strict_config_value])
        # Prefer the options explicitly specified in the command line
        #
        # TODO: There is a bug that `system_config.log.rotate_age/rotate_size` are
        # not merged with the command line options since they are not in
        # `SYSTEM_CONFIG_PARAMETERS`.
        # We have to fix this bug.
        opt = {}
        Fluent::SystemConfig::SYSTEM_CONFIG_PARAMETERS.each do |param|
        if @cl_opt.key?(param) && !@cl_opt[param].nil?
        opt[param] = @cl_opt[param]
        end
        end
        system_config.overwrite_variables(**opt)
        system_config
        end

As with #4836, it is sufficient for now to pass the configuration value to ServerEngine.

@egemenkus
Copy link
Author

Alright, thank you. I will wait and try to contribute again after reviewing it in more detail.

@daipom
Copy link
Contributor

daipom commented Feb 21, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daipom daipom daipom requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

System config: add umask option
2 participants
@egemenkus @daipom